Directly start foreground service #7340

tobiasKaminsky · 2020-11-17T11:08:50Z

Fix #2010
Fix #6905

This is not done at all, but it prevents from crashing on my test device.
@ezaquarii for some reason the startForeground in PlayerService is not called, when starting a video.
Thus it get killed by Android after a specific time.

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

Testing

Writing tests is very important. Please try to write some tests for your PR.
If you need help, please do not hesitate to ask in this PR for help.

unit tests
instrumented tests
UI tests

Tests written, or not not needed

ezaquarii · 2020-11-17T11:12:31Z

@tobiasKaminsky this is very strange edge case as video player does not use PlayerService at all - it depends on a media player in VideoView.

It sounds to me like the player service might be receiving unintended events from GUI.

InfamousUser · 2020-11-17T14:15:26Z

Tried it, it does not seem to crash, but I think crashing for me is caused by autoupload, which doesn't seem to want to start on this app. I did turn off doze for it. Maybe it needs a bit of time...

InfamousUser · 2020-11-17T18:42:12Z

Ok, autoupload started working, it took a while to initialize and well...

The problem is fixed. The problem being the app crash, now the app doesn't crash but instead the whole app or Android system freezes and/or ANR dialogue is shown. This fixed one of the symptoms apparently. Looks like the underlying problem was just exacerbating the crash case, I did not know there was an underlying problem in the first place.

Okay, so the problem for me is app freezing.
Also the ANR dialogue is different now, there's only 2 options whereas there were 3 before.

P.S. Can't even open the app anymore - it stops responding on the initial loading screen.

src/main/java/com/nextcloud/client/media/PlayerService.kt

tobiasKaminsky · 2020-11-18T07:25:23Z

@tobiasKaminsky this is very strange edge case as video player does not use PlayerService at all - it depends on a media player in VideoView.

It sounds to me like the player service might be receiving unintended events from GUI.

I was confused by that first, too.
When you click on an image it opens PreviewImageFragment
When you click on an audio/video it opens PreviewMediaFragment

PreviewMediaFragment then can do both: play audio or video.
If audio plays and you click "back", then audio continues to play in background (via PlayerService).
I guess that this is then the reason why PlayerService is started.

When you then click on video in PreviewMediaFragment it opens up PreviewVideoActivity to display video in fullscreen…

tobiasKaminsky · 2020-11-18T07:41:26Z

Reason why PlayerService is started is:

android/src/main/java/com/owncloud/android/ui/preview/PreviewMediaFragment.java

Lines 326 to 329 in 77751da

    
           } else if (MimeTypeUtil.isVideo(file)) { 
        
               stopAudio(); 
        
               playVideo(); 
        
           }

Which is somehow needed:

play audio
go back -> audio continues in background
start a video

tobiasKaminsky · 2020-11-18T07:49:03Z

@InfamousUser can you test it again, please?

tobiasKaminsky · 2020-11-18T07:49:42Z

@ezaquarii I found the cause and hopefully a proper fix. Can you review it? :-)

nextcloud-android-bot · 2020-11-18T08:05:28Z

master-light-blue-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/16785-Screenshot-master-light-blue

nextcloud-android-bot · 2020-11-18T08:08:30Z

stable-light-blue-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/16785-Screenshot-stable-light-blue

nextcloud-android-bot · 2020-11-18T08:23:40Z

master-dark-blue-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/16785-Screenshot-master-dark-blue

nextcloud-android-bot · 2020-11-18T08:27:07Z

stable-dark-blue-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/16785-Screenshot-stable-dark-blue

InfamousUser · 2020-11-18T13:47:37Z

@InfamousUser can you test it again, please?

Ok, autoupload started working, it took a while to initialize and well...

The problem is fixed. The problem being the app crash, now the app doesn't crash but instead the whole app or Android system freezes and/or ANR dialogue is shown. This fixed one of the symptoms apparently. Looks like the underlying problem was just exacerbating the crash case, I did not know there was an underlying problem in the first place.

Okay, so the problem for me is app freezing.
Also the ANR dialogue is different now, there's only 2 options whereas there were 3 before.

P.S. Can't even open the app anymore - it stops responding on the initial loading screen.

Probably same as above, I can't test because of the original problem. The app is frozen solid on startup for 80 seconds (loading screen), then displays frozen main interface.

It also locks the Android interface half the times for up to half a minute, that shouldn't be possible, probably a bug in Android.

tobiasKaminsky · 2020-11-19T07:27:37Z

Probably same as above, I can't test because of the original problem. The app is frozen solid on startup for 80 seconds (loading screen), then displays frozen main interface.

It also locks the Android interface half the times for up to half a minute, that shouldn't be possible, probably a bug in Android.

Can you create another issue for this?
Please also try to get logcat.

tobiasKaminsky · 2020-11-19T07:27:54Z

/backport to stable-3.14

InfamousUser · 2020-11-19T15:38:09Z

Not sure whether you wanted the log here or in the new issue, but here it is here just in case. Also not sure this is what you mean by logcat, never used it before, this is what I got by finding an app called that.

Nextcloud.txt

InfamousUser · 2020-11-19T15:47:13Z

New issue: #7378

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky · 2020-11-20T08:39:50Z

❗ Rebased ❗

nextcloud-android-bot · 2020-11-20T08:45:54Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/7340.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

nextcloud-android-bot · 2020-11-20T08:46:29Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/7340.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

nextcloud-android-bot · 2020-11-20T08:47:10Z

master-Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/16857-Unit-master-08-45

codecov · 2020-11-20T09:03:07Z

Codecov Report

Merging #7340 (343561a) into master (843ecb4) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #7340      +/-   ##
============================================
- Coverage     29.13%   29.10%   -0.04%     
  Complexity        5        5              
============================================
  Files           442      442              
  Lines         34823    34840      +17     
  Branches       4817     4824       +7     
============================================
- Hits          10147    10140       -7     
- Misses        23182    23206      +24     
  Partials       1494     1494

Impacted Files	Coverage Δ	Complexity Δ
...n/java/com/nextcloud/client/media/PlayerService.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...cloud/android/ui/preview/PreviewMediaFragment.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...cloud/android/datamodel/UploadsStorageManager.java	`66.30% <0.00%> (-1.82%)`	`0.00% <0.00%> (ø%)`
...oud/android/operations/RefreshFolderOperation.java	`75.27% <0.00%> (-1.46%)`	`0.00% <0.00%> (ø%)`
...ain/java/com/owncloud/android/db/UploadResult.java	`39.74% <0.00%> (-1.29%)`	`0.00% <0.00%> (ø%)`
.../owncloud/android/files/services/FileUploader.java	`49.49% <0.00%> (-0.21%)`	`0.00% <0.00%> (ø%)`
...ncloud/android/operations/UploadFileOperation.java	`60.26% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
...oud/android/ui/activity/SyncedFoldersActivity.java	`25.67% <0.00%> (+0.98%)`	`0.00% <0.00%> (ø%)`

nextcloud-android-bot · 2020-11-20T09:08:45Z

Codacy

Lint

Type	master	PR
Warnings	307	307
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	27
Correctness Warnings	52
Internationalization Warnings	9
Multithreaded correctness Warnings	9
Performance Warnings	72
Security Warnings	41
Dodgy code Warnings	102
Total	312

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	27
Correctness Warnings	52
Internationalization Warnings	9
Multithreaded correctness Warnings	9
Performance Warnings	72
Security Warnings	41
Dodgy code Warnings	102
Total	312

nextcloud-android-bot · 2020-11-20T09:10:21Z

stable-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/16857-IT-stable-09-08

Directly start foreground service Signed-off-by: thelittlefireman <thelittlefireman@users.noreply.github.com>

tobiasKaminsky added the 2. developing label Nov 17, 2020

tobiasKaminsky requested a review from ezaquarii November 17, 2020 11:08

This was referenced Nov 17, 2020

Crash due to PlayerService #6905

Closed

Context.startForegroundService() did not then call Service.startForeground() #2010

Closed

ezaquarii reviewed Nov 17, 2020

View reviewed changes

src/main/java/com/nextcloud/client/media/PlayerService.kt Outdated Show resolved Hide resolved

tobiasKaminsky force-pushed the foregroundServiceCrash branch from 41dcb04 to 8f17962 Compare November 18, 2020 07:48

nextcloud deleted a comment from nextcloud-android-bot Nov 18, 2020

tobiasKaminsky mentioned this pull request Nov 18, 2020

Replace full-screen background when viewing films #6470

Closed

tobiasKaminsky mentioned this pull request Nov 19, 2020

Crash on playing videos #7364

Closed

tobiasKaminsky requested a review from ezaquarii November 19, 2020 07:27

tobiasKaminsky added 3. to review and removed 2. developing labels Nov 19, 2020

backportbot-nextcloud bot added the backport-request label Nov 19, 2020

InfamousUser mentioned this pull request Nov 19, 2020

Autoupload causes Nextcloud app to freeze and not respond after crashing is fixed #7378

Open

aqaqsubin mentioned this pull request Nov 20, 2020

App crash #7379

Closed

tobiasKaminsky added 2 commits November 20, 2020 09:39

Fix falsely starting player service when previewing video

be22388

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

Add toggle function

343561a

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky force-pushed the foregroundServiceCrash branch from 1af8e41 to 343561a Compare November 20, 2020 08:39

aqaqsubin mentioned this pull request Nov 20, 2020

NC Crashes while watching Sample video via app #7394

Closed

tobiasKaminsky requested a review from ezaquarii November 20, 2020 13:05

AndyScherzinger merged commit 9a80066 into master Nov 20, 2020

delete-merged-branch bot deleted the foregroundServiceCrash branch November 20, 2020 20:27

AndyScherzinger added this to the Nextcloud App 3.15.0 milestone Nov 20, 2020

backportbot-nextcloud bot mentioned this pull request Nov 20, 2020

[stable-3.14] Directly start foreground service #7409

Merged

backportbot-nextcloud bot removed the backport-request label Nov 20, 2020

aqaqsubin mentioned this pull request Nov 23, 2020

Consistant crash on playing video #7396

Closed

This was referenced Nov 23, 2020

Error #7457

Closed

Crash #7456

Closed

This was referenced Nov 24, 2020

Crash when switching profiles #7463

Closed

Cannot Play/Stream Video on Android App #7460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directly start foreground service #7340

Directly start foreground service #7340

tobiasKaminsky commented Nov 17, 2020 •

edited

Loading

ezaquarii commented Nov 17, 2020

InfamousUser commented Nov 17, 2020 •

edited

Loading

InfamousUser commented Nov 17, 2020

tobiasKaminsky commented Nov 18, 2020

tobiasKaminsky commented Nov 18, 2020

tobiasKaminsky commented Nov 18, 2020

tobiasKaminsky commented Nov 18, 2020

nextcloud-android-bot commented Nov 18, 2020

nextcloud-android-bot commented Nov 18, 2020

nextcloud-android-bot commented Nov 18, 2020

nextcloud-android-bot commented Nov 18, 2020

InfamousUser commented Nov 18, 2020 •

edited

Loading

tobiasKaminsky commented Nov 19, 2020

tobiasKaminsky commented Nov 19, 2020

InfamousUser commented Nov 19, 2020

InfamousUser commented Nov 19, 2020

tobiasKaminsky commented Nov 20, 2020

nextcloud-android-bot commented Nov 20, 2020

nextcloud-android-bot commented Nov 20, 2020

nextcloud-android-bot commented Nov 20, 2020

codecov bot commented Nov 20, 2020 •

edited

Loading

nextcloud-android-bot commented Nov 20, 2020

nextcloud-android-bot commented Nov 20, 2020

Directly start foreground service #7340

Directly start foreground service #7340

Conversation

tobiasKaminsky commented Nov 17, 2020 • edited Loading

Testing

ezaquarii commented Nov 17, 2020

InfamousUser commented Nov 17, 2020 • edited Loading

InfamousUser commented Nov 17, 2020

tobiasKaminsky commented Nov 18, 2020

tobiasKaminsky commented Nov 18, 2020

tobiasKaminsky commented Nov 18, 2020

tobiasKaminsky commented Nov 18, 2020

nextcloud-android-bot commented Nov 18, 2020

nextcloud-android-bot commented Nov 18, 2020

nextcloud-android-bot commented Nov 18, 2020

nextcloud-android-bot commented Nov 18, 2020

InfamousUser commented Nov 18, 2020 • edited Loading

tobiasKaminsky commented Nov 19, 2020

tobiasKaminsky commented Nov 19, 2020

InfamousUser commented Nov 19, 2020

InfamousUser commented Nov 19, 2020

tobiasKaminsky commented Nov 20, 2020

nextcloud-android-bot commented Nov 20, 2020

nextcloud-android-bot commented Nov 20, 2020

nextcloud-android-bot commented Nov 20, 2020

codecov bot commented Nov 20, 2020 • edited Loading

Codecov Report

nextcloud-android-bot commented Nov 20, 2020

Codacy

Lint

SpotBugs (new)

SpotBugs (master)

nextcloud-android-bot commented Nov 20, 2020

tobiasKaminsky commented Nov 17, 2020 •

edited

Loading

InfamousUser commented Nov 17, 2020 •

edited

Loading

InfamousUser commented Nov 18, 2020 •

edited

Loading

codecov bot commented Nov 20, 2020 •

edited

Loading